Skip to content

RFC: Make WeakRef cheaply cloneable#1045

Draft
heftig wants to merge 1 commit into
gtk-rs:mainfrom
heftig:weakref-arc
Draft

RFC: Make WeakRef cheaply cloneable#1045
heftig wants to merge 1 commit into
gtk-rs:mainfrom
heftig:weakref-arc

Conversation

@heftig

@heftig heftig commented Mar 6, 2023

Copy link
Copy Markdown
Contributor

By using an Arc instead of a Box, we can make WeakRef::clone a cheap operation, compared to taking a global write lock for setting a GWeakRef. The extra space used for the refcounts should be a fine trade-off.

@heftig heftig changed the title glib: Make WeakRef cheaply cloneable RFC: Make WeakRef cheaply cloneable Mar 6, 2023
@heftig heftig force-pushed the weakref-arc branch 2 times, most recently from 2cabe2c to d32746b Compare March 6, 2023 20:53
By using an `Arc` instead of a `Box`, we can make `WeakRef::clone` a
cheap operation, compared to taking a global write lock for setting a
`GWeakRef`. The extra space used for the refcounts should be a fine
trade-off.
@heftig

heftig commented Mar 6, 2023

Copy link
Copy Markdown
Contributor Author

Using weakref.set(&obj) will now affect not just weakref but all of its clones, which is probably surprising. Do we maybe want a ArcWeakRef variant that cannot be set?

Comment thread glib/src/object.rs
}
}

unsafe impl<T: ObjectType + Sync + Sync> Sync for WeakRef<T> {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Can you make a separate PR for this?

@sdroege

sdroege commented Mar 7, 2023

Copy link
Copy Markdown
Member

Using weakref.set(&obj) will now affect not just weakref but all of its clones, which is probably surprising. Do we maybe want a ArcWeakRef variant that cannot be set?

Yes that seems problematic. Also did you consider improving the situation in C by moving this to GObjectPrivate with a per-object mutex?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants